Skip to content

Conversation

tmiasko
Copy link
Contributor

@tmiasko tmiasko commented Jun 4, 2021

otherwise evaluation could change after further substitutions.

@rust-highfive
Copy link
Contributor

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

@rust-highfive
Copy link
Contributor

r? @varkor

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 4, 2021
@RalfJung
Copy link
Member

RalfJung commented Jun 4, 2021

Thanks, this makes sense.
Is there any reason not to do this for all eval_nullary_intrinsic?

@RalfJung
Copy link
Member

RalfJung commented Jun 4, 2021

Cc @oli-obk

@tmiasko
Copy link
Contributor Author

tmiasko commented Jun 4, 2021

Is there any reason not to do this for all eval_nullary_intrinsic?

I could be useful for optimizations to evaluate variant_count::<Option<T>>() or needs_drop::<ManuallyDrop<T>>() as soon as possible.

I do have a test case, but wasn't sure if we want one that depends on const_evaluatable_unchecked; following compiles on nightly:

#![feature(const_generics)]

struct Value<const B: bool> {}
impl Value<true> { fn is_true() {} }

fn f<T>() {
    Value::<{std::mem::needs_drop::<T>()}>::is_true();
}
fn main() {
    f::<u32>();
}

@RalfJung
Copy link
Member

RalfJung commented Jun 4, 2021

I could be useful for optimizations to evaluate variant_count::<Option>() or needs_drop::<ManuallyDrop>() as soon as possible.

Well, for the latter that won't happen any more with this PR, right?
If we want this, we need a needs_drop inside rustc that can answer "yes", "no", and "don't know".

@tmiasko
Copy link
Contributor Author

tmiasko commented Jun 4, 2021

Well, for the latter that won't happen any more with this PR, right?

For variant_count we already have suitable implementation, for needs_drop we would need to change it.

@RalfJung
Copy link
Member

RalfJung commented Jun 5, 2021

Ah, fair.

Then maybe add comments to the other branches (the ones that do not call "ensure_monomorphic") explicitly saying "this code will correctly handle non-monomorphic calls" or so. I want to make sure that when/if another intrinsic is added, we don't introduce the same bug again...

@tmiasko tmiasko force-pushed the monomorphic-needs-drop branch from 136ae04 to e3e6cb2 Compare June 5, 2021 10:51
@varkor
Copy link
Member

varkor commented Jun 5, 2021

r? @RalfJung

@rust-highfive rust-highfive assigned RalfJung and unassigned varkor Jun 5, 2021
@RalfJung
Copy link
Member

RalfJung commented Jun 5, 2021

Code change looks good to me; is there any way to have a test for this? r=me otherwise.

@tmiasko tmiasko force-pushed the monomorphic-needs-drop branch from e3e6cb2 to 07a03b0 Compare June 5, 2021 16:29
@tmiasko
Copy link
Contributor Author

tmiasko commented Jun 5, 2021

Added a test case mentioned earlier.

@RalfJung
Copy link
Member

RalfJung commented Jun 5, 2021

Awesome, thanks a lot. :)
@bors r+

@bors
Copy link
Collaborator

bors commented Jun 5, 2021

📌 Commit 07a03b0 has been approved by RalfJung

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 5, 2021
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jun 5, 2021
…alfJung

Disallow non-monomorphic calls to `needs_drop` in interpreter

otherwise evaluation could change after further substitutions.
@@ -0,0 +1,20 @@
error[E0599]: no function or associated item named `assert` found for struct `Bool<{ std::mem::needs_drop::<T>() }>` in the current scope
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error is useless - it would be good to suppress it. Is there already an issue for this kind of error?

@bors
Copy link
Collaborator

bors commented Jun 11, 2021

⌛ Testing commit 07a03b0 with merge 66ba810...

@bors
Copy link
Collaborator

bors commented Jun 11, 2021

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing 66ba810 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 11, 2021
@bors bors merged commit 66ba810 into rust-lang:master Jun 11, 2021
@rustbot rustbot added this to the 1.54.0 milestone Jun 11, 2021
@tmiasko tmiasko deleted the monomorphic-needs-drop branch June 11, 2021 10:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants